The handler function accepts a list of parameters as described in its OpenAPI specification.

See invokeHandler below and our docs: https://loopback.io/doc/en/lb4/Routes.html#using-partial-openapi-spec-fragments

You cannot access the request/response from a handler route (a Route instance). In my comment #1784 (comment), I was showing how to create a new kind of RouteEntry (a new class implementing RouteEntry interface). RouteEntry's method invokeHandler does have access to the request/response objects.

  // somewhere in the code setting up RoutingTable
  // the anonymous RouteEntry object should be refactored into a class
  this.routingTable.catchAllRoute = <RouteEntry>{
    verb: 'get',
    path: '*',
    spec: { /* TODO: this route should be excluded from the spec */ },
    updateBindings(requestContext: Context) {
      // no-op
    },
    
    invokeHandler(
      {request, response}: Context,
      args: OperationArgs,
    ): Promise<OperationRetval> {
      return this._handleStaticAssetRequest(request, response);
    }

    describe(): string {
      return "final route to handle static assets";
    }
  };

Let's use context.request and context.response, it should be faster than obtaining the values via Context bindings.

I am proposing to simplify the design here. Since there will be always at most one StaticRoute instance registered with a server, there is no need to make the handler configurable. Let's move the code dealing with _routerForStaticAssets here into invokeHandler.

Ideally, I'd like to see _routerForStaticAssets to be a member property of StaticRoute.

Let's be more verbose here. See how we deal with 404 errors now:

https://github.com/strongloop/loopback-next/blob/a2ab86d3a3c721f28159585ca4e87bd8504476c4/packages/rest/src/router/routing-table.ts#L183-L186

IIUC, this is relying on the fact that the code paths invoked here is bypassing path validations and that our routing table will pass the path * to Express router without modifications.

I find that rather brittle, let's find a different solution please.

I am thinking about the following implementation of RoutingTable#find:

class RoutingTable {
  // ...

  find(request: Request): ResolvedRoute {
    debug('Finding route %s for %s %s', request.method, request.path);

    const found = this._router.find(request);

    if (found) {
      debug('Route matched: %j', found);
      return found;
    }

    debug('No API route found for %s %s, trying to find a static assets', request.method, request.path);
    return this._staticAssetsRoute;
  }
}

Let's call this StaticAssetsRoute. The name "static route" can be also interpreted as a route that's static (e.g. does not accept any path parameters).

This is a bit hacky to test path === '*'. The special route is also added to the underlying router, which might cause unexpected results.

@raymondfeng you had some reservations about this.

This is a bit hacky to test path === '*'. The special route is also added to the underlying router, which might cause unexpected results.

Can you elaborate the concerns, so that we can start a discussion?

On line 162, the special route is added to the router implementation, which probably doesn't know /*.

I am still not able to see what the potential problem is. We run validateApiPath(route.path) and /* is a valid key to the static assets router. What am I missing?

It's not helpful to have /* in the router which won't match any requests.

In your implementation, the static asset route is a special one outside of the Router responsibility. It serves as the last matching candidate.

So do we use a non-path key, like path = 'catch-all-router'? We'll have to skip path validation in that case. What are your suggestions?

@bajtos what are your thoughts on this?

I'm fine to have /* as the path value, but don't call this._router.add(route);.

Got it now.

ExpressRouter is used in rest.server.ts and routing-table.ts, and defined in both the files. What is a good location to export it from to avoid duplication?

Having to extend express.Router because its definition file does not include the handle method for it. I wonder why.

Please note that express.Router itself is the handle function.

src/types.ts is a good place to define common interfaces

As @raymondfeng said, express.Router is a callable function. It extends IRouter which extends RequestHandler which is defined as follows:

export interface RequestHandler {
    // tslint:disable-next-line callable-types (This is extended from and can't extend from a type alias in ts<2.2
    (req: Request, res: Response, next: NextFunction): any;
}

I don't think this is a valid cast as express.Router() returns a function which is a handler.

As @raymondfeng said, express.Router is a callable function. It extends IRouter which extends RequestHandler which is defined as follows:

export interface RequestHandler {
    // tslint:disable-next-line callable-types (This is extended from and can't extend from a type alias in ts<2.2
    (req: Request, res: Response, next: NextFunction): any;
}

Let's move the implementation details of dealing with static assets down to StaticAssetsRoute please.

Ideally, I'd like to see RestServer#static implemented as a thin wrapper delegating the registration of static-asset middleware to StaticAssetsRoute and/or our HttpHandler class.

See also my earlier #1848 (comment)

@bajtos Calling app.static after the server has started should still set up the static assets route successfully. I tried to understand what you are suggesting and moved some pieces around, but resulted in app.static losing functionality after the app started.

Can you elaborate more on your suggestion, please? What does the outline of RestServer#static look like?

Let's wait until you have addressed the other comments and then I can try to shuffle the code around myself, I think it will be the fastest option.

I had addressed all others, except this one. Please refresh to see the changes.

When the router called us back, then please remove onFinished listener to avoid resolving/rejecting the promise twice and also to avoid a potential (memory) leak.

It's a once, do we still need to remove it?

Yes, because when the static asset cannot be found or a ReadStream cannot be created, then the finish event is not fired within the timeframe of this async method.

I guess one could argue that the finish method will be eventually fired because the HTTP response will be finished by other parts of the framework eventually, but I find it very brittle and hacky to rely on some other part of our codebase to trigger "finish". See also Action at a distance anti-pattern.

No need to use async if you are not calling await.

Have you considered inlining this._handler inside invokeHandler?

class StaticAssetsRoute /*...*/ {
//...

  invokeHandler(
    {request, response}: RequestContext,
    args: OperationArgs,
  ): Promise<OperationRetval> {
    return new Promise((resolve, reject) => {
      const onFinished = () => resolve();
      response.once('finish', onFinished);
     // ...
    });
  }

//...
}

I am proposing to rephrase this test name as follows:

it('gives precedence to API routes over static assets')